Skip to content

fix(mercury): fix memory recorder for delta objects#756

Merged
genedna merged 4 commits into
gitmono-dev:mainfrom
el-ev:fix_memory_monitoring
Dec 15, 2024
Merged

fix(mercury): fix memory recorder for delta objects#756
genedna merged 4 commits into
gitmono-dev:mainfrom
el-ev:fix_memory_monitoring

Conversation

@el-ev

@el-ev el-ev commented Dec 14, 2024

Copy link
Copy Markdown
Contributor

Sorry for the disturbance. This patch should preserve the functionality of memory recorder while improving the performance.

Context: #755 (comment)

@vercel

vercel Bot commented Dec 14, 2024

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mega ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 15, 2024 3:39am

Comment thread mercury/src/internal/pack/decode.rs
Comment thread mercury/src/internal/pack/cache_object.rs Outdated
@el-ev

el-ev commented Dec 15, 2024

Copy link
Copy Markdown
Contributor Author

@MrBeanCpp Uh, upon further digging, I found that for memory limits from 1GiB to 6GiB (on my end, couldn't be larger because decoding ends earlier than the memory fills up), the threshold in decode is never reached:

while self.pool.queued_count() > 2000 
    || self.mem_limit.map(|limit| self.memory_used() > limit).unwrap_or(false) {
    thread::yield_now();
}

It's always the case that the 4/5 cache limit is hit and obj memory stops increasing. Do you have any idea on this?

@MrBeanCpp

Copy link
Copy Markdown
Contributor

We have set a limit on pool.queued_count() (2000), which causes us to stop data input before memory_used() reaches its limit.

Is this what you meant? This is also for memory control, as the number of tasks in the thread pool affects memory usage and is difficult to estimate. Therefore, a certain task limit is necessary and will not affect speed.

@el-ev

el-ev commented Dec 15, 2024

Copy link
Copy Markdown
Contributor Author

We have set a limit on pool.queued_count() (2000), which causes us to stop data input before memory_used() reaches its limit.

Is this what you meant? This is also for memory control, as the number of tasks in the thread pool affects memory usage and is difficult to estimate. Therefore, a certain task limit is necessary and will not affect speed.

No I'm talking about the LRU Cache, whose capacity is set to 4/5 of total memory limit. It always reaches the (4/5 total) limit before memory_used() does, making the following check always fail:

self.mem_limit.map(|limit| self.memory_used() > limit).

@MrBeanCpp

Copy link
Copy Markdown
Contributor

That's okay. Not a big problem.

Even in this situation, you can see that the total memory usage has exceeded the limit slightly through the Task Manager.

@Hou-Xiaoxuan

Copy link
Copy Markdown
Contributor

@el-ev The decoding and caching logic includes compromises to manage memory effectively. Achieving precise memory control is a highly complex task. Previously, there was already an implementation for decoding, but the primary reason for rewriting this part was to manage memory usage effectively and prevent the decoding process from triggering an OOM (Out of Memory) error. Hardcoded values like 5/4 and 2000 were added incrementally during debugging to optimize memory usage.

@genedna
genedna added this pull request to the merge queue Dec 15, 2024
Merged via the queue into gitmono-dev:main with commit d805656 Dec 15, 2024
@el-ev

el-ev commented Dec 15, 2024

Copy link
Copy Markdown
Contributor Author

@el-ev The decoding and caching logic includes compromises to manage memory effectively. Achieving precise memory control is a highly complex task. Previously, there was already an implementation for decoding, but the primary reason for rewriting this part was to manage memory usage effectively and prevent the decoding process from triggering an OOM (Out of Memory) error. Hardcoded values like 5/4 and 2000 were added incrementally during debugging to optimize memory usage.

I see, it is some kind of heuristics.

@el-ev
el-ev deleted the fix_memory_monitoring branch December 15, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants